-
Notifications
You must be signed in to change notification settings - Fork 12
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Embedded contribution form #325
Conversation
Thanks ! Can you rebase on |
src/app/content/App/CreateNoticeScreen/CreateNoticeScreen.stories.tsx
Outdated
Show resolved
Hide resolved
src/app/content/App/PublishedNoticeScreen/PublishedNoticeScreen.stories.tsx
Outdated
Show resolved
Hide resolved
src/components/molecules/IntentionsSelector/IntentionsSelectorContainer.ts
Outdated
Show resolved
Hide resolved
ea1785a
to
f018647
Compare
I'm opening PRs directly in this branch to build upon your work @newick .... |
Actually, as @lutangar spotted in #332 we missed the contributor I added a comment in the Trello card: https://trello.com/c/wrkki02b/142-embedded-contribution-form-sendinblue-mailjet |
8464bf2
to
72b1fdb
Compare
72b1fdb
to
5aad906
Compare
04ba976
to
6dfff98
Compare
@bmenant we need a last functional review on this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some code comments ...
src/app/lmem/Errors.ts
Outdated
@@ -0,0 +1,3 @@ | |||
export default interface Errors { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this is FormError
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one is in our domain, FormErrors
is not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Except that, to me, what you are describing is a form error, not a general LMEM error.
Then maybe this should be outside app/lmem
and be called FormErrors
to be correct, no ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
src/app/lmem/contribution/validate.ts
and src/app/lmem/contribution/validate.ts
can be used outside of a form context, but I could move validateForm.ts
outside of lmem though.
The idea was to propose an Errors
interface for validation errors of a "model". The name could be better though, smthing like ValidationErrors
or ModelErrors
or ModelValidationErrors
. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I see what you're trying to do. Maybe ValidationErrors
makes it more explicit then.
a9422dd
to
675f72e
Compare
@@ -1,4 +1,4 @@ | |||
import { PersistedState } from 'redux-persist/es/types'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WTF I just had this problem in another branch when attempting to push.... isn't that supposed to be in develop?
@@ -90,22 +89,28 @@ export const getFormRegisteredFields = (formName: string) => ( | |||
return R.path(['registeredFields'], form) || []; | |||
}; | |||
|
|||
const getFieldPathErrorMessage = (state: ContentState, formName: string) => ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
}, {}) | ||
.filter(errorMessage => errorMessage); | ||
}; | ||
): string[] => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
expect(validateContribution()).to.not.be.empty; | ||
}); | ||
it('rejects empty contribution', () => { | ||
// @ts-ignore |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So we can still use ts-ignore after all?
54ba871
to
978ecfc
Compare
add intention selector change cheap selector to efficient ones
add form elements add intention selector change cheap selector to efficient ones add `onChange` handler on `IntentionSelector`
…ection handle contribution form 1. Submit: sync validation only 2. Preview: show server error when failing 3. Submitted: removed the button to see the notice
fix preview overflow and background issues
…view screen if submission failed
try to clarify getFlatFormErrors fix typings issues functional style to validateContribution
978ecfc
to
b95b018
Compare
🎉 This PR is included in version 3.0.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Screens for https://trello.com/c/wrkki02b/142-embedded-contribution-form-sendinblue-mailjet exist.
Time to add some logic @lutangar @JalilArfaoui :)